Skip to content

{Core} Honor scopes specified by Track 2 SDK#15184

Merged
jiasli merged 11 commits intoAzure:devfrom
jiasli:get_token
Oct 27, 2020
Merged

{Core} Honor scopes specified by Track 2 SDK#15184
jiasli merged 11 commits intoAzure:devfrom
jiasli:get_token

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Sep 16, 2020 •
edited
Loading

Fix #15179

Issue

In

  • Track 1 SDK, scopes (resource) is managed by Azure CLI
  • Track 2 SDK, scopes (resource) is managed by SDK

For example, for Track 2 SDK mgmt-plane SubscriptionClientConfiguration:

self.credential_scopes = ['https://management.azure.com/.default']

For Track 2 SDK data-plane AzureAppConfigurationClient:

if aad_mode:
    scope = base_url.strip("/") + "/.default"

But in AdalAuthentication.get_token, scopes is not honored (discarded), resulting in getting a token for a wrong scopes (ARM https://management.core.windows.net/ by default):

azure-cli/src/azure-cli-core/azure/cli/core/adal_authentication.py

Lines 61 to 67 in c2b9a2c

# This method is exposed for Azure Core.
def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
_, token, full_token, _ = self._get_token()
try:
return AccessToken(token, int(full_token['expiresIn'] + time.time()))
except KeyError: # needed to deal with differing unserialized MSI token payload
return AccessToken(token, int(full_token['expires_on']))

Fix

This PR fixes the issue by honoring the scopes specified by Track 2 SDK.

Testing Guide

User Identity

# Track 1 SDK mgmt-plan
> az group show -n fy --debug
DEBUG: AdalAuthentication.signed_session invoked by Track 1 SDK

# Track 2 SDK mgmt-plane
> az account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590 --debug
DEBUG: AdalAuthentication.get_token invoked by Track 2 SDK with scopes=('https://management.azure.com/.default',)

# Track 1 SDK data-plane
> az account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590 --debug
DEBUG: Profile.get_raw_token invoked with resource='https://storage.azure.com', subscription=None, tenant=None

# Track 2 SDK data-plane
az storage blob list --account-name st0921 -c con1 --auth-mode login --debug
DEBUG: AdalAuthentication.get_token invoked by Track 2 SDK with scopes=('https://storage.azure.com/.default',)

Managed Identity

# Track 1 SDK mgmt-plan
> az group show -n fy --debug
DEBUG: MSIAuthenticationWrapper.signed_session invoked by Track 1 SDK

# Track 2 SDK mgmt-plane
> az account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590 --debug
DEBUG: MSIAuthenticationWrapper.get_token invoked by Track 2 SDK with scopes=('https://management.azure.com/.default',)

# Track 1 SDK data-plane
> az account subscription show --id 0b1f6471-1bf0-4dda-aec3-cb9272f09590 --debug
DEBUG: Profile.get_raw_token invoked with resource='https://storage.azure.com', subscription=None, tenant=None

# Track 2 SDK data-plane
az storage blob list --account-name st0921 -c con1 --auth-mode login --debug
DEBUG: MSIAuthenticationWrapper.get_token invoked by Track 2 SDK with scopes=('https://storage.azure.com/.default',)

@yonzhan yonzhan added this to the S176 milestone Sep 16, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Sep 16, 2020

Core

jiasli
jiasli commented Sep 21, 2020
View reviewed changes
src/azure-cli/azure/cli/command_modules/storage/_validators.py
profile = Profile(cli_ctx=cmd.cli_ctx)
n.token_credential, _, _ = profile.get_login_credentials(
resource="https://storage.azure.com", subscription_id=n._subscription)
n.token_credential, _, _ = profile.get_login_credentials(subscription_id=n._subscription)
Copy link
Member Author

@jiasli jiasli Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope is now managed by Track 2 Storage SDK (azure/multiapi/storagev2/blob/v2019_12_12/_shared/constants.py:25):

STORAGE_OAUTH_SCOPE = "https://storage.azure.com/.default"

@jiasli jiasli marked this pull request as ready for review September 21, 2020 10:07
@jiasli
Copy link
Member Author

jiasli commented Sep 24, 2020

Unfortunately, due an incorrect credential_scopes managing logic in existing SDKs (Azure/azure-sdk-for-python#12947), even though the bug has been fixed in Azure/autorest.python#745, merging this PR will break all commands using Track 2 SDKs with sovereign clouds, as credential_scopes will be passed to scopes as

['https://management.azure.com/.default', 'https://management.core.chinacloudapi.cn/.default']

'https://management.core.chinacloudapi.cn/.default' will be discarded.

jiasli
jiasli commented Sep 24, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/adal_authentication.py
Comment on lines +71 to +76
# Deal with an old Track 2 SDK issue where the default credential_scopes is extended with
# custom credential_scopes. Instead, credential_scopes should be replaced by custom credential_scopes.
# https://github.com/Azure/azure-sdk-for-python/issues/12947
# We simply remove the first one if there are multiple scopes provided.
if len(scopes) > 1:
scopes = scopes[1:]
Copy link
Member Author

@jiasli jiasli Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a patch to handle issue Azure/azure-sdk-for-python#12947 in old Track 2 SDKs.

@yonzhan yonzhan modified the milestones: S176, S177 Sep 27, 2020
@jiasli jiasli mentioned this pull request Oct 9, 2020
Merged
2 tasks
@jiasli jiasli requested a review from evelyn-ys as a code owner October 16, 2020 08:45
@yonzhan yonzhan modified the milestones: S177, S178 Oct 24, 2020
jiasli added 2 commits October 26, 2020 10:33
@jiasli
Merge branch 'dev' into get_token
aa2696d 
# Conflicts:
#	src/azure-cli-core/azure/cli/core/adal_authentication.py
@jiasli
style
c08ef44
@jiasli jiasli requested a review from bim-msft October 26, 2020 09:20
arrownj
arrownj reviewed Oct 26, 2020
View reviewed changes
jsntcy
jsntcy reviewed Oct 26, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/util.py
:rtype: str
"""
scope = scopes[0]
if scope.endswith(".default"):
Copy link
Member

@jsntcy jsntcy Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if scope.endswith(".default"): [](start = 4, length = 30)

Do we need make this case insensitive?

Copy link
Member Author

@jiasli jiasli Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. .default is the only supported form. See The /.default scope.

jsntcy
jsntcy reviewed Oct 26, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/util.py

# Trim extra ending slashes. https://datalake.azure.net// -> https://datalake.azure.net/
scope = scope.rstrip('/') + '/'
return scope
Copy link
Member

@jsntcy jsntcy Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it expected that "scope" is https://management.core.windows.net/ or it should be https://management.core.windows.net?

Copy link
Member Author

@jiasli jiasli Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. See

azure-cli/src/azure-cli-core/azure/cli/core/cloud.py

Line 303 in 4e1ff0e

active_directory_resource_id='https://management.core.windows.net/',

jsntcy
jsntcy reviewed Oct 26, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/util.py
:return: The ADAL resource
:rtype: str
"""
scope = scopes[0]
Copy link
Member

@jsntcy jsntcy Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only support one scope now as it is a list?

Copy link
Member Author

@jiasli jiasli Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Request an access token,

The scopes must all be from a single resource, along with OIDC scopes (profile, openid, email)

jsntcy
jsntcy reviewed Oct 26, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/util.py
:param resource: The ADAL resource ID
:return: A list of scopes
"""
if 'datalake' in resource or 'batch' in resource or 'database' in resource:
Copy link
Member

@jsntcy jsntcy Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that we move this tricky logic from core to module level?

Copy link
Member Author

@jiasli jiasli Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Currently resource_to_scopes is only called by _get_mgmt_service_client. We should actually remove this tricky logic totally from this function. Will do in a separate PR.

jsntcy
jsntcy reviewed Oct 26, 2020
View reviewed changes
jsntcy
jsntcy approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jiasli jiasli merged commit a804e5f into Azure:dev Oct 27, 2020
@jiasli jiasli mentioned this pull request Oct 29, 2020
Merged
@avanigupta avanigupta mentioned this pull request Oct 30, 2020
Merged
2 tasks
@jiasli jiasli mentioned this pull request Nov 5, 2020
Merged
@jiasli jiasli deleted the get_token branch November 5, 2020 09:44
@jiasli jiasli added the Track2 label Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@jsntcy jsntcy jsntcy approved these changes

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@Juliehzl Juliehzl Awaiting requested review from Juliehzl

@fengzhou-msft fengzhou-msft Awaiting requested review from fengzhou-msft

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

@mmyyrroonn mmyyrroonn Awaiting requested review from mmyyrroonn

+2 more reviewers

@arrownj arrownj arrownj approved these changes

@bim-msft bim-msft bim-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiasli jiasli

Labels

Track2

Projects

None yet

Milestone

S178

Development

Successfully merging this pull request may close these issues.

{Core} AdalAuthentication doesn't honor scopes for Track 2 SDK

7 participants

@jiasli @yonzhan @arrownj @bim-msft @evelyn-ys @jsntcy @zhoxing-ms